Skip to content

DEPR: Deprecate sort=None for union and implement sort=True #25980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

reidy-p
Copy link
Contributor

@reidy-p reidy-p commented Apr 3, 2019

This is a WIP pull request because I wanted to make sure I'm going in the right direction before continuing.

In #24959 it was agreed that the default for some of the set operations should be sort=None for backwards compatibility in 0.24.1. sort=None generally tries to sort in all cases except when the two indices are equal or one of the indices is empty. This meant that in a future release we could introduce a proper sort=True which would try to sort in all cases and change the default to sort=False in all cases.

Are there any potential problems with this plan?

In this PR I have started to deprecate sort=None for Index.union and to introduce sort=True. sort=None is currently the default for this set operation so a FutureWarning will be given by default. I will make similar changes to the union method in all the index classes if this is the approach we want to take.

expected = i1.union(i2, sort=sort).union(i3, sort=sort)

warning = FutureWarning if sort is None else None
with tm.assert_produces_warning(warning):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would just be easier to move the warnings to a separate test since this will eventually be ripped out anyway

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #25980 into master will decrease coverage by 49.91%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #25980       +/-   ##
===========================================
- Coverage   91.82%   41.91%   -49.92%     
===========================================
  Files         175      175               
  Lines       52540    52562       +22     
===========================================
- Hits        48247    22030    -26217     
- Misses       4293    30532    +26239
Flag Coverage Δ
#multiple ?
#single 41.91% <61.53%> (+1.04%) ⬆️
Impacted Files Coverage Δ
pandas/core/indexes/base.py 55.75% <61.53%> (-41.05%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.16%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.1%) ⬇️
pandas/core/tools/numeric.py 10.44% <0%> (-89.56%) ⬇️
... and 134 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0610a60...923b4d2. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #25980 into master will decrease coverage by 50.06%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #25980       +/-   ##
===========================================
- Coverage   91.87%   41.81%   -50.07%     
===========================================
  Files         180      174        -6     
  Lines       50859    50711      -148     
===========================================
- Hits        46726    21204    -25522     
- Misses       4133    29507    +25374
Flag Coverage Δ
#multiple ?
#single 41.81% <61.53%> (-0.18%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/base.py 55.65% <61.53%> (-41.6%) ⬇️
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/io/gcs.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/s3.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.16%) ⬇️
... and 152 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b115a6b...2286cd1. Read the comment docs.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you merge master when you have a chance

@@ -0,0 +1 @@
/Users/paul/anaconda/bin/activate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like some extra files got included

@reidy-p reidy-p force-pushed the sort_true_union branch from 3d8be67 to 2286cd1 Compare July 1, 2019 20:38
@WillAyd
Copy link
Member

WillAyd commented Aug 28, 2019

Thanks for the PR @reidy-p ! Looks like this has gone stale though, so closing to clean up our queue. Please ping if you'd like to pick it back up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants